Add support for Error::Fatal by kfarnung · Pull Request #70 · nodejs/node-addon-api

kfarnung · 2017-06-30T00:13:46Z

Added static method Error::Fatal to invoke napi_fatal_error
Added node_internals.cc/h to shim missing internal functions
Added a test for Error::Fatal

kfarnung · 2017-06-30T00:15:33Z

/cc @digitalinfinity @jasongin @gabrielschulhof

Ignore the changes to node_api.cc/h for now as they include the in-progress changes from nodejs/node#13971. I also need to use napi_fatal_error in a few locations throughout the wrapper code, but haven't gotten there yet.

jasongin · 2017-06-30T00:33:07Z

test/index.js

    args.splice(0, 0, '--napi-modules');
  }
-  const child = require('child_process').spawnSync(process.argv[0], args, {
+  const child = require('./napi_child').spawnSync(process.argv[0], args, {


Did you intend to delete the splicing lines above?

jasongin · 2017-06-30T00:38:41Z

src/node_internals.h

+
+#ifndef SRC_NODE_INTERNALS_H_
+#define SRC_NODE_INTERNALS_H_
+


Can you put a comment in here explaining that this is not the real, complete "node_internals", but just a shim for building node_api.cc outside of node?

bnoordhuis · 2017-06-30T09:11:41Z

napi-inl.h

 #endif // NAPI_CPP_EXCEPTIONS

+#define NAPI_FATAL_IF_FAILED(status, location, message) \
+  if ((status) != napi_ok) Error::Fatal((location), (message));


Suggestion: wrap this in a do { ... } while so that if (...) NAPI_FATAL_IF_FAILED(...); else ... does the right thing.

bnoordhuis · 2017-06-30T09:14:03Z

src/node_api.cc

+    v8::ObjectTemplate::New(isolate);
+  wrapperTemplate->SetInternalFieldCount(1);
+  v8::Local<v8::Object> wrapper =
+    wrapperTemplate->NewInstance(context).ToLocalChecked();


Style: indent line continuations by four spaces, not two.

bnoordhuis · 2017-06-30T09:23:58Z

src/node_api.cc


-  obj->SetInternalField(0, v8::External::New(isolate, native_object));
+  // Create a wrapper object with an internal field to hold the wrapped pointer.
+  v8::Local<v8::ObjectTemplate> wrapperTemplate =


Style: wrapper_template (snake_case, not camelCase, for locals.)

Substance: creating an ObjectTemplate every time leaks memory. V8 caches them internally and it doesn't put an upper bound or a lifetime on elements in the cache. Create a single ObjectTemplate and reuse that.

Hmm ... we do this upstream as well.

bnoordhuis · 2017-06-30T09:25:19Z

src/node_api.cc

+  // Insert the wrapper into the object's prototype chain.
+  v8::Local<v8::Value> proto = obj->GetPrototype();
+  wrapper->SetPrototype(proto);
+  obj->SetPrototype(wrapper);


Can you use the Maybe overloads here?

bnoordhuis · 2017-06-30T09:31:55Z

src/node_internals.h

@@ -0,0 +1,90 @@
+// Copyright Joyent, Inc. and other Node contributors.


None of the code in this file is (c) Joyent, it's all (c) yours truly. You can leave out the copyright boilerplate or reduce it to just 'Node contributors.'

The same applies to the preceding file, I think: it's a mix of code authored by @mscdex and me (and maybe @indutny.)

Thanks @bnoordhuis, I was looking for some guidance here. I'll change the copyright headers.

bnoordhuis · 2017-06-30T09:32:49Z

test/error.js

+    process.execPath, [ __filename, 'fatal', bindingPath ]);
+  assert.ifError(p.error);
+  assert.ok(p.stderr.toString().includes(
+    'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));


Style: 4 space indent for line continuations.

V8 caches and does not subsequently release `ObjectTemplate` instances. Thus, we need to store the `ObjectTemplate` from which we derive object instances we use for `napi_wrap()` and function/accessor context in a persistent in the `napi_env`. nodejs/node-addon-api#70 (comment)

V8 caches and does not subsequently release `ObjectTemplate` instances. Thus, we need to store the `ObjectTemplate` from which we derive object instances we use for `napi_wrap()` and function/accessor context in a persistent in the `napi_env`. nodejs/node-addon-api#70 (comment) PR-URL: #13999 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co> Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>

jasongin · 2017-07-12T22:24:28Z

@kfarnung Is this ready to merge? It looks like you've addressed @bnoordhuis's feedback.

kfarnung · 2017-07-12T22:26:23Z

I was waiting on the upstream change (nodejs/node#13971) to land before trying to get this merged.

mhdawson · 2017-07-14T14:46:57Z

I guess this now needs to be rebased to remove the changes to the node files.

kfarnung · 2017-07-14T16:08:25Z

Yup, I'll take care of that momentarily.

mhdawson

LGTM

* Added static method `Error::Fatal` to invoke `napi_fatal_error` * Added `node_internals.cc/h` to shim missing internal functions * Added a test for `Error::Fatal` * Replaced usage of assert with calls to `Error::Fatal`

kfarnung · 2017-07-14T18:52:05Z

Rebased one last time and new CI run: https://travis-ci.org/kfarnung/node-addon-api/builds/253725282

mhdawson · 2017-07-14T18:57:58Z

Looks like Ben's comments have been addressed so landing.

* Added static method `Error::Fatal` to invoke `napi_fatal_error` * Added `node_internals.cc/h` to shim missing internal functions * Added a test for `Error::Fatal` * Replaced usage of assert with calls to `Error::Fatal` PR-URL: #70 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson · 2017-07-14T19:00:08Z

landed as 28fad43

V8 caches and does not subsequently release `ObjectTemplate` instances. Thus, we need to store the `ObjectTemplate` from which we derive object instances we use for `napi_wrap()` and function/accessor context in a persistent in the `napi_env`. nodejs/node-addon-api#70 (comment) PR-URL: #13999 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co> Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>

V8 caches and does not subsequently release `ObjectTemplate` instances. Thus, we need to store the `ObjectTemplate` from which we derive object instances we use for `napi_wrap()` and function/accessor context in a persistent in the `napi_env`. nodejs/node-addon-api#70 (comment) PR-URL: nodejs#13999 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co> Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>

V8 caches and does not subsequently release `ObjectTemplate` instances. Thus, we need to store the `ObjectTemplate` from which we derive object instances we use for `napi_wrap()` and function/accessor context in a persistent in the `napi_env`. nodejs/node-addon-api#70 (comment) Backport-PR-URL: #19447 PR-URL: #13999 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.co> Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>

* Added static method `Error::Fatal` to invoke `napi_fatal_error` * Added `node_internals.cc/h` to shim missing internal functions * Added a test for `Error::Fatal` * Replaced usage of assert with calls to `Error::Fatal` PR-URL: nodejs/node-addon-api#70 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasongin reviewed Jun 30, 2017

View reviewed changes

jasongin approved these changes Jun 30, 2017

View reviewed changes

bnoordhuis reviewed Jun 30, 2017

View reviewed changes

gabrielschulhof mentioned this pull request Jun 30, 2017

N-API: Reuse ObjectTemplate used for napi_wrap() nodejs/node#13999

Closed

2 tasks

kfarnung force-pushed the node_internal branch from 010ed9d to a48525f Compare July 5, 2017 17:52

jasongin mentioned this pull request Jul 12, 2017

Update to new error helper signatures in n-api #78

Closed

kfarnung force-pushed the node_internal branch from a48525f to 24f90ee Compare July 12, 2017 22:27

kfarnung mentioned this pull request Jul 12, 2017

Update files from node #79

Closed

kfarnung force-pushed the node_internal branch from 24f90ee to 7b9f7bd Compare July 12, 2017 23:04

kfarnung force-pushed the node_internal branch from 7b9f7bd to 685c215 Compare July 14, 2017 16:17

mhdawson approved these changes Jul 14, 2017

View reviewed changes

Add support for Error::Fatal

a4aaeaa

* Added static method `Error::Fatal` to invoke `napi_fatal_error` * Added `node_internals.cc/h` to shim missing internal functions * Added a test for `Error::Fatal` * Replaced usage of assert with calls to `Error::Fatal`

kfarnung force-pushed the node_internal branch from 685c215 to a4aaeaa Compare July 14, 2017 18:50

mhdawson closed this Jul 14, 2017

kfarnung deleted the node_internal branch July 14, 2017 19:02

jasongin mentioned this pull request Jul 17, 2017

Abort with fatal error when error-handling fails #48

Closed

		@@ -0,0 +1,90 @@
		// Copyright Joyent, Inc. and other Node contributors.


		#ifndef SRC_NODE_INTERNALS_H_
		#define SRC_NODE_INTERNALS_H_

Uh oh!

Conversation

kfarnung commented Jun 30, 2017

Uh oh!

kfarnung commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasongin commented Jul 12, 2017

Uh oh!

kfarnung commented Jul 12, 2017

Uh oh!

mhdawson commented Jul 14, 2017

Uh oh!

kfarnung commented Jul 14, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

kfarnung commented Jul 14, 2017

Uh oh!

mhdawson commented Jul 14, 2017

Uh oh!

mhdawson commented Jul 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants